-
Notifications
You must be signed in to change notification settings - Fork 121
[POS as a tab i2] Refresh POS eligibility in ineligible UI: site settings and feature switch disabled cases #15895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ityCheckerProtocol` with basic implementation in `POSTabEligibilityChecker`.
|
|
joshheald
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well, 🚢 – thanks!
| func syncSiteSettingsRemotely() async throws { | ||
| try await withCheckedThrowingContinuation { [weak self] (continuation: CheckedContinuation<Void, Error>) in | ||
| guard let self else { | ||
| return continuation.resume(throwing: POSTabEligibilityCheckerError.selfDeallocated) | ||
| } | ||
| stores.dispatch(SettingAction.synchronizeGeneralSiteSettings(siteID: siteID) { [weak self] error in | ||
| guard let self else { | ||
| return continuation.resume(throwing: POSTabEligibilityCheckerError.selfDeallocated) | ||
| } | ||
| if let error { | ||
| return continuation.resume(throwing: error) | ||
| } | ||
| siteSettings.refresh() | ||
| continuation.resume(returning: ()) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we could/should start defining these sorts of things in Yosemite? It's not as good as full async, but better than potentially writing this much nested code more than once. The errors would have to be generic, and we might need to pass in stores, but that seems potentially OK?
Not thought this through particularly thoroughly though. There's a good chance we'd be better off investing time in making Yosemite more async friendly. Nothing to do in this PR 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Definitely would be nice to create better interface in Yosemite for the POS use case, though sometimes there's already quite some logic in the Yosemite Store implementation. In general, I've been creating new APIs in Yosemite if it's something that doesn't exactly exist in a Yosemite Store. For something that's already available in a Yosemite Store, I still reuse it.
Would like to get your thought on the naming of the new APIs in Yosemite. WDYT about creating POS{Entity}Service for {Entity}Store in Yosemite? Currently, we have POSOrderService, POSReceiptService, POSEligibilityService behind their own protocol in Yosemite. I can make this change separately.
|
Merging this first to unblock other refresh behavior work, will likely make changes separately for #15895 (comment). |

For WOOMOB-757
Just one review is required.
Description
This pull request introduces a new method
refreshEligibility(ineligibleReason:)toPOSEntryPointEligibilityCheckerProtocolwith basic implementation inPOSTabEligibilityCheckerto handle eligibility state updates based on specific ineligible reasons. Site settings related cases and the POS feature switch disabled case are covered in this PR. Unsupported WC plugin version case will be supported separately in WOOMOB-799.Eligibility refresh logic:
WooCommerce/Classes/POS/Controllers/POSEntryPointController.swift: Updated therefreshEligibilitymethod to accept anineligibleReasonparameter and useposEligibilityCheckerfor eligibility state updates.WooCommerce/Classes/POS/Presentation/PointOfSaleEntryPointView.swift: Modified thePOSIneligibleViewrefresh action to pass thereasonparameter to theposEntryPointController.refreshEligibilitymethod.Protocol and implementation updates:
WooCommerce/Classes/ViewRelated/Dashboard/Settings/POS/POSTabEligibilityChecker.swift: Added therefreshEligibilitymethod to thePOSEntryPointEligibilityCheckerProtocoland implemented it inPOSTabEligibilityCheckerto handle various ineligible reasons, including syncing site settings remotely and checking eligibility again.checkEligibilitycurrently just includes one API request to reload the POS feature switch value, the site settings and WC plugin values are synced outside of the eligibility checker from site initialization. [1] [2]Error handling and site settings synchronization:
WooCommerce/Classes/ViewRelated/Dashboard/Settings/POS/POSTabEligibilityChecker.swift: Introduced thePOSTabEligibilityCheckerErrorenum for error handling, including aselfDeallocatedcase. Added thesyncSiteSettingsRemotelymethod to synchronize site settings and refresh eligibility state. [1] [2]Legacy checker adjustments:
WooCommerce/Classes/ViewRelated/Dashboard/Settings/POS/LegacyPOSTabEligibilityChecker.swift: Added arefreshEligibilitymethod with an assertion failure for unsupported refresh operations in the legacy POS tab implementation.Steps to reproduce
Unsupported store currency
Prerequisite: for a store that is eligible for POS, change the currency in core under WC settings > General to a different value
Retry--> the CTA should be in loading state, then back to normal stateRetry--> the CTA should be in loading state, then the app enters POSPOS feature switch disabled
Prerequisite: for a store that is eligible for POS and with WC version 10.0+, disable the POS feature in WooCommerce settings > Advanced > Features
Retry--> the CTA should be in loading state, then back to normal stateRetry--> the CTA should be in loading state, then the app enters POSTesting information
I tested the above cases, and when the device is offline.
Screenshots
Unsupported store currency
Simulator.Screen.Recording.-.iPad.A16.-.2025-07-10.at.15.15.40.mp4
POS feature switch disabled
Simulator.Screen.Recording.-.iPad.A16.-.2025-07-10.at.15.20.10.mp4
RELEASE-NOTES.txtif necessary.